packages/apps-engine/src/definition/version.ts (2)

1-11: Trim the leading doc block per coding guidelines.

The repo guideline for **/*.{ts,tsx,js} is to avoid code comments in the implementation. The rationale here is genuinely non-obvious (compile-time vs runtime path resolution), so consider keeping at most a one-line note next to the require() call and dropping the rest, or moving the explanation to the PR/commit description.

As per coding guidelines: "Avoid code comments in the implementation".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/apps-engine/src/definition/version.ts` around lines 1 - 11, Remove
the long leading doc block and replace it with a single-line comment immediately
adjacent to the require() that reads package.json (e.g. "// Use require() so TS
doesn't resolve the path at compile time"), keeping the actual code (the
require() call and exported version constant) unchanged; move the detailed
explanation into the PR/commit message instead of the source.

---

16-16: Consider freezing the dependency on a moving package shape.

require(requirePath).version will silently produce undefined (typed as string) if the field is ever missing or the file is renamed. Since the whole point of this module is to fail loudly when version detection breaks, a small assertion would be cheap and would surface misconfiguration immediately at module load:

🛡️ Suggested guard

-// eslint-disable-next-line import/no-dynamic-require, `@typescript-eslint/no-require-imports` -- Paths are bounded, we just need to decide which package.json file to target
-export const ENGINE_VERSION: string = require(requirePath).version;
+// eslint-disable-next-line import/no-dynamic-require, `@typescript-eslint/no-require-imports` -- Paths are bounded, we just need to decide which package.json file to target
+const { version } = require(requirePath) as { version?: string };
+if (typeof version !== 'string') {
+    throw new Error(`Apps-Engine: failed to resolve version from ${requirePath}`);
+}
+export const ENGINE_VERSION: string = version;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/apps-engine/src/definition/version.ts` at line 16, The current
export ENGINE_VERSION uses require(requirePath).version which can silently be
undefined; update the module so after loading the package JSON via
require(requirePath) (using the existing requirePath symbol) you assert that the
loaded object has a non-empty version property and throw a clear error if not
present, then assign that value to ENGINE_VERSION; ensure the runtime check
happens at module load so any missing/renamed field fails loudly with a helpful
message mentioning requirePath and "version".

packages/apps-engine/src/server/compiler/AppPackageParser.ts (1)

10-10: Optional: drop the redundant instance field.

appsEngineVersion is now just a copy of the module-level ENGINE_VERSION constant — the per-instance field, the explicit string annotation, and the indirection at the call sites no longer add value. Using the constant directly makes the dependency more obvious and removes one layer of state on the parser.

♻️ Proposed simplification

-	private appsEngineVersion: string = ENGINE_VERSION;
-
@@
-		if (!semver.satisfies(this.appsEngineVersion, info.requiredApiVersion)) {
-			throw new RequiredApiVersionError(info, this.appsEngineVersion);
+		if (!semver.satisfies(ENGINE_VERSION, info.requiredApiVersion)) {
+			throw new RequiredApiVersionError(info, ENGINE_VERSION);
 		}

Skip if you want to preserve the field as a stable seam for future test injection.

Also applies to: 18-18

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/apps-engine/src/server/compiler/AppPackageParser.ts` at line 10, The
instance field appsEngineVersion on AppPackageParser is redundant because it
just mirrors the module-level ENGINE_VERSION; remove the appsEngineVersion
declaration and any constructor assignment, then change all call sites inside
AppPackageParser that reference this.appsEngineVersion to use ENGINE_VERSION
directly (retain import of ENGINE_VERSION). Ensure you also remove the explicit
string type annotation and any related tests or references that assume the
per-instance field.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/apps-engine/src/definition/version.ts (1)

12-13: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Make runtime path detection cross-platform.

Line 12 uses a POSIX-suffix check; Windows __dirname uses \, so runningFromSource can be false and Line 13 can resolve the wrong package.json.

Suggested fix

+import path from 'node:path';
+
-const runningFromSource = __dirname.endsWith('src/definition');
+const runningFromSource =
+	path.basename(path.dirname(__dirname)) === 'src' &&
+	path.basename(__dirname) === 'definition';
 const requirePath = runningFromSource ? '../../package.json' : '../package.json';

#!/bin/bash
python - <<'PY'
paths = [
    r"C:\repo\packages\apps-engine\src\definition",
    "/repo/packages/apps-engine/src/definition",
]
for p in paths:
    print(f"{p} -> endsWith('src/definition') = {p.endswith('src/definition')}")
PY

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/apps-engine/src/definition/version.ts` around lines 12 - 13, The
runtime path detection uses a POSIX string suffix on __dirname (const
runningFromSource) which fails on Windows; change the check to a cross-platform
comparison using Node's path utilities (path.normalize/path.join or splitting by
path.sep) to compare the last path segments (e.g., 'src' and 'definition') and
then set requirePath accordingly; update the symbols runningFromSource,
requirePath and reference __dirname when implementing the normalized/joined
comparison so the correct package.json is resolved on all platforms.

packages/apps-engine/src/definition/version.ts (1)

1-11: ⚡ Quick win

Remove the implementation block comment in this module.

Please drop or relocate this explanatory block to PR/docs to keep implementation code comment-free per repo guidance.

Suggested cleanup

-/**
- * The version of the Apps-Engine package.
- * Consumed by host-side code (e.g. AppPackageParser) to validate app compatibility
- * without relying on filesystem path traversal.
- *
- * Uses require() instead of a static import so TypeScript does not resolve the path
- * at compile time.
- *
- * When running for tests, using ts-node, package.json is located two levels above the current file.
- * When running in production, package.json is located one level above the compiled version of this file.
- */
 const runningFromSource = __dirname.endsWith('src/definition');

As per coding guidelines: "Avoid code comments in the implementation."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/apps-engine/src/definition/version.ts` around lines 1 - 11, Remove
the large explanatory block comment at the top of the module (the multi-line
comment describing Apps-Engine package version, require() rationale, and
test/production package.json locations); relocate that explanation into the PR
description or repository docs and leave this file free of implementation
comments so only the actual module code (the version export/require logic)
remains in packages/apps-engine/src/definition/version.ts.